Update cluster config#154
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughStorage initialization refactored to support dynamic Data Store (DS) topology resolution via peer fetch or local generation; solo-mode bootstrap handling added; Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Init
participant Bootstrap as Bootstrap Logic
participant ClusterCfg as Cluster Config Reader
participant Peer as Peer Fetch
participant LocalGen as Local Config Gen
participant DSS as DataStoreService
Init->>Bootstrap: opt_bootstrap ?
rect rgb(230,245,255)
Note over Bootstrap: Solo-mode path (node 0 local IP/port)
Bootstrap->>ClusterCfg: construct solo ng_config
end
Init->>ClusterCfg: not bootstrap -> ReadClusterConfigFile()/parse NgConfig lists
Init->>Peer: ds_peer_node provided?
alt peer provided
Peer->>Peer: FetchConfigFromPeer()
Peer-->>Init: topology (or abort on failure)
else no peer
Init->>LocalGen: TxConfigsToDssClusterConfig(node_id, native_ng_id, ng_configs)
LocalGen-->>Init: topology (validate multi-node clusters)
end
Init->>DSS: StartService(leader_id, node_id, topology_config)
DSS-->>Init: started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
storage/eloq/ha_eloq.cc(3 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
storage/eloq/ha_eloq.cc
🔇 Additional comments (2)
storage/eloq/tx_service (1)
1-1: Verify the submodule commit update aligns with PR objectives.The
tx_servicesubmodule pointer has been advanced to a new commit, but without visibility into the actual changes within that submodule, it's unclear whether this update is related to the cluster config restructuring mentioned in the PR objectives or if it contains breaking changes.Please confirm:
- What changes are included in the updated
tx_servicecommit (5cd5be353827a58263b21e1118e37209c00c4cad)?- How does this update relate to the cluster config initialization restructuring in
ha_eloq.cc?- Are there any API or interface changes in
tx_servicethat affect the DSS bootstrap/cluster-config logic?storage/eloq/store_handler (1)
1-1: Verify submodule commit validity and compatibility with DSS initialization changes.This is a submodule pointer update without visible code context. The commit hash is being changed from
62e17d3b8c8c4095d094b66351fdd7dec3a4f4d4to8d8d1b3a9d08cc53b07910e2f80cf0002df2a2be.Given that the PR also restructures DSS initialization in
ha_eloq.cc, ensure that:
- The new store_handler commit is compatible with the updated DSS initialization logic
- No breaking changes are introduced in the store_handler commit
- The submodule update is necessary for the bootstrap/cluster-config handling introduced elsewhere in this PR
Please confirm that this submodule update aligns with the PR objectives and verify compatibility with the DSS changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/eloq/ha_eloq.cc (1)
2663-2663: Fix create_if_missing gating; honor actual topology
is_single_nodeis hardcoded to true, so StartService runs with create_if_missing=true even in multi-node setups. This risks conflicting initialization.Apply:
- bool is_single_node= true; + bool is_single_node = (ng_configs.size() == 1);No call-site change needed; this will correctly drive:
ret = data_store_service_->StartService((opt_bootstrap || is_single_node), dss_leader_id, dss_node_id);Also applies to: 2831-2833
♻️ Duplicate comments (1)
storage/eloq/ha_eloq.cc (1)
2790-2803: Abort on DSS peer fetch failure; don’t return successReturning
falsehere returns 0 fromeloq_init_func, incorrectly signaling success and skipping cleanup. Route through the plugin’s abort path and use server logging for consistency.Apply:
- if (!EloqDS::DataStoreService::FetchConfigFromPeer(eloq_dss_peer_node, - ds_config)) - { - LOG(ERROR) << "Failed to fetch config from peer node: " - << eloq_dss_peer_node; - return false; - } + if (!EloqDS::DataStoreService::FetchConfigFromPeer(eloq_dss_peer_node, + ds_config)) + { + sql_print_error("Failed to fetch DSS config from peer node: %s", + eloq_dss_peer_node); + DBUG_RETURN(eloq_init_abort()); + }
🧹 Nitpick comments (1)
storage/eloq/ha_eloq.cc (1)
2681-2733: Remove large commented-out legacy DSS config blockThe legacy file-based DSS config code is commented out, adding noise and future-maintenance risk. Delete it; git history preserves it.
Apply:
- // std::string dss_config_file_path= eloq_dss_config_file_path; - // if (dss_config_file_path.empty()) - // { - // dss_config_file_path= dss_data_path + "/dss_config.ini"; - // } - // ... - // else - // { - // sql_print_information("EloqDataStoreService loaded config file %s", - // dss_config_file_path.c_str()); - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
storage/eloq/ha_eloq.cc(3 hunks)storage/eloq/store_handler(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/eloq/store_handler
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
storage/eloq/ha_eloq.cc
🔇 Additional comments (1)
storage/eloq/ha_eloq.cc (1)
2593-2615: Verify native_ng_id assignment for non-candidate members
native_ng_idis only set whenis_candidate_is true. If the current node is not marked candidate but belongs to a node group,native_ng_idremains 0 and is later used for DS/Tx service startup and config derivation. Please confirm this is intended. If not, setnative_ng_id = pair.firstwhen the local node matches, independent of candidacy.Option:
- if (ng_nodes[i].is_candidate_) - { - // found native_ng_id. - native_ng_id= pair.first; - break; - } + // Record native group id when membership is found. + native_ng_id = pair.first; + if (ng_nodes[i].is_candidate_) { + // Prefer breaking early if this node can be leader. + break; + }
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
storage/eloq/ha_eloq.cc (1)
2725-2763: Abort initialization on DSS peer fetch failure; don’t return success pathCurrent code returns false inside eloq_init_func (an int), which equals 0/success and skips the plugin abort/cleanup. This leaves the server thinking init succeeded while DSS topology didn’t load.
Apply this diff:
- if (!EloqDS::DataStoreService::FetchConfigFromPeer(eloq_dss_peer_node, - ds_config)) - { - LOG(ERROR) << "Failed to fetch config from peer node: " - << eloq_dss_peer_node; - return false; - } + if (!EloqDS::DataStoreService::FetchConfigFromPeer(eloq_dss_peer_node, + ds_config)) + { + sql_print_error("Failed to fetch DSS config from peer node: %s", + eloq_dss_peer_node); + DBUG_RETURN(eloq_init_abort()); + }Additionally, the local variable masks the sysvar:
- dss_config_file_path is hardcoded to "", ignoring MYSQL_SYSVAR(dss_config_file_path). Wire the sysvar:
- std::string dss_config_file_path= ""; + std::string dss_config_file_path = + (eloq_dss_config_file_path && std::strlen(eloq_dss_config_file_path) > 0) + ? std::string(eloq_dss_config_file_path) + : std::string();
🧹 Nitpick comments (1)
storage/eloq/ha_eloq.cc (1)
2725-2763: Enforce peer-node requirement only for truly multi-node deploymentsYou abort when ng_configs.size() > 1 without peer, but a single group with multiple nodes also needs a peer. Consider validating the total node count instead:
- if (ng_configs.size() > 1) + size_t ds_nodes_total = 0; + for (const auto &kv : ng_configs) ds_nodes_total += kv.second.size(); + if (ds_nodes_total > 1) { sql_print_error("EloqDS multi-node cluster must specify " "eloq_dss_peer_node."); DBUG_RETURN(eloq_init_abort()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
storage/eloq/build_tx_service.cmake(2 hunks)storage/eloq/ha_eloq.cc(2 hunks)storage/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/eloq/tx_service
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
storage/eloq/build_tx_service.cmakestorage/eloq/ha_eloq.cc
🔇 Additional comments (1)
storage/eloq/build_tx_service.cmake (1)
204-204: Verify the necessity and platform compatibility of linking the atomic library for tx_service.The
tx_servicemodule is a git submodule that wasn't initialized in the sandbox, preventing verification of whether it actually uses atomic operations. The unconditional linking ofatomicon line 204 warrants confirmation:
- Verify actual usage: Check if tx_service source code uses
std::atomicor atomic operations that require the library.- Check platform handling: Review whether other similar targets in the codebase use conditional linking for the atomic library (e.g., only on platforms where it's required).
- Confirm necessity: Determine if this is a hard requirement or a legacy inclusion from an older submodule version, especially on modern systems where atomics may be compiler built-ins.
If atomic is confirmed necessary, consider adding platform-specific logic to prevent linking failures on systems without libatomic.
| if (opt_bootstrap) | ||
| { | ||
| // When execute mysql_install_db.sh, eloq should run in solo mode. | ||
| std::vector<NodeConfig> solo_config; | ||
| solo_config.emplace_back(0, local_ip, local_port); | ||
| ng_configs.try_emplace(0, std::move(solo_config)); | ||
| } | ||
| else if (!txservice::ReadClusterConfigFile(cluster_config_file, ng_configs, | ||
| cluster_config_version)) | ||
| { | ||
|
|
||
| // Read cluster topology from general config file in this case | ||
| auto parse_res= txservice::ParseNgConfig( | ||
| eloq_ip_list, eloq_standby_ip_list, eloq_voter_ip_list, ng_configs, | ||
| eloq_node_group_replica_num, 0); | ||
| if (!parse_res) | ||
| { | ||
| LOG(ERROR) << "Failed to extract cluster configs from ip_port_list."; | ||
| DBUG_RETURN(eloq_init_abort()); | ||
| } | ||
| } | ||
|
|
||
| bool found= false; | ||
| uint32_t native_ng_id= 0; | ||
| // check whether this node is in cluster. | ||
| for (auto &pair : ng_configs) | ||
| { | ||
| auto &ng_nodes= pair.second; | ||
| for (size_t i= 0; i < ng_nodes.size(); i++) | ||
| { | ||
| if (ng_nodes[i].host_name_ == local_ip && | ||
| ng_nodes[i].port_ == local_port) | ||
| { | ||
| node_id= ng_nodes[i].node_id_; | ||
| found= true; | ||
| if (ng_nodes[i].is_candidate_) | ||
| { | ||
| // found native_ng_id. | ||
| native_ng_id= pair.first; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!found) | ||
| { | ||
| sql_print_error("!!!!!!!! Current node does not belong to any node " | ||
| "group, EloqDB " | ||
| "startup is terminated !!!!!!!!"); | ||
| DBUG_RETURN(eloq_init_abort()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix native_ng_id assignment and stop scanning once the local node is found
- native_ng_id is set only when is_candidate_ is true; for non-candidate (e.g., standby/learner) nodes it remains 0. This can misconfigure TxService/DSS when this node is part of a group but not a candidate.
- The outer loop doesn’t break after a match; we keep scanning needlessly and risk overwriting state.
Apply this diff to always capture the group id and exit both loops after the first match:
- bool found= false;
- uint32_t native_ng_id= 0;
+ bool found= false;
+ uint32_t native_ng_id= 0;
// check whether this node is in cluster.
- for (auto &pair : ng_configs)
+ for (const auto &pair : ng_configs)
{
auto &ng_nodes= pair.second;
for (size_t i= 0; i < ng_nodes.size(); i++)
{
if (ng_nodes[i].host_name_ == local_ip &&
ng_nodes[i].port_ == local_port)
{
node_id= ng_nodes[i].node_id_;
- found= true;
- if (ng_nodes[i].is_candidate_)
- {
- // found native_ng_id.
- native_ng_id= pair.first;
- break;
- }
+ native_ng_id= pair.first; // set regardless of candidate role
+ found= true;
+ break;
}
}
+ if (found) break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opt_bootstrap) | |
| { | |
| // When execute mysql_install_db.sh, eloq should run in solo mode. | |
| std::vector<NodeConfig> solo_config; | |
| solo_config.emplace_back(0, local_ip, local_port); | |
| ng_configs.try_emplace(0, std::move(solo_config)); | |
| } | |
| else if (!txservice::ReadClusterConfigFile(cluster_config_file, ng_configs, | |
| cluster_config_version)) | |
| { | |
| // Read cluster topology from general config file in this case | |
| auto parse_res= txservice::ParseNgConfig( | |
| eloq_ip_list, eloq_standby_ip_list, eloq_voter_ip_list, ng_configs, | |
| eloq_node_group_replica_num, 0); | |
| if (!parse_res) | |
| { | |
| LOG(ERROR) << "Failed to extract cluster configs from ip_port_list."; | |
| DBUG_RETURN(eloq_init_abort()); | |
| } | |
| } | |
| bool found= false; | |
| uint32_t native_ng_id= 0; | |
| // check whether this node is in cluster. | |
| for (auto &pair : ng_configs) | |
| { | |
| auto &ng_nodes= pair.second; | |
| for (size_t i= 0; i < ng_nodes.size(); i++) | |
| { | |
| if (ng_nodes[i].host_name_ == local_ip && | |
| ng_nodes[i].port_ == local_port) | |
| { | |
| node_id= ng_nodes[i].node_id_; | |
| found= true; | |
| if (ng_nodes[i].is_candidate_) | |
| { | |
| // found native_ng_id. | |
| native_ng_id= pair.first; | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| if (!found) | |
| { | |
| sql_print_error("!!!!!!!! Current node does not belong to any node " | |
| "group, EloqDB " | |
| "startup is terminated !!!!!!!!"); | |
| DBUG_RETURN(eloq_init_abort()); | |
| } | |
| if (opt_bootstrap) | |
| { | |
| // When execute mysql_install_db.sh, eloq should run in solo mode. | |
| std::vector<NodeConfig> solo_config; | |
| solo_config.emplace_back(0, local_ip, local_port); | |
| ng_configs.try_emplace(0, std::move(solo_config)); | |
| } | |
| else if (!txservice::ReadClusterConfigFile(cluster_config_file, ng_configs, | |
| cluster_config_version)) | |
| { | |
| // Read cluster topology from general config file in this case | |
| auto parse_res= txservice::ParseNgConfig( | |
| eloq_ip_list, eloq_standby_ip_list, eloq_voter_ip_list, ng_configs, | |
| eloq_node_group_replica_num, 0); | |
| if (!parse_res) | |
| { | |
| LOG(ERROR) << "Failed to extract cluster configs from ip_port_list."; | |
| DBUG_RETURN(eloq_init_abort()); | |
| } | |
| } | |
| bool found= false; | |
| uint32_t native_ng_id= 0; | |
| // check whether this node is in cluster. | |
| for (const auto &pair : ng_configs) | |
| { | |
| auto &ng_nodes= pair.second; | |
| for (size_t i= 0; i < ng_nodes.size(); i++) | |
| { | |
| if (ng_nodes[i].host_name_ == local_ip && | |
| ng_nodes[i].port_ == local_port) | |
| { | |
| node_id= ng_nodes[i].node_id_; | |
| native_ng_id= pair.first; // set regardless of candidate role | |
| found= true; | |
| break; | |
| } | |
| } | |
| if (found) break; | |
| } | |
| if (!found) | |
| { | |
| sql_print_error("!!!!!!!! Current node does not belong to any node " | |
| "group, EloqDB " | |
| "startup is terminated !!!!!!!!"); | |
| DBUG_RETURN(eloq_init_abort()); | |
| } |
🤖 Prompt for AI Agents
In storage/eloq/ha_eloq.cc around lines 2571-2623, the code only assigns
native_ng_id when ng_nodes[i].is_candidate_ is true and continues scanning after
finding the local node which can leave native_ng_id unset for non-candidate
nodes and risk overwriting state; change the logic so that when a local node
match is found you always set node_id, found = true and native_ng_id =
pair.first (regardless of is_candidate_), then break out of the inner loop and
also break the outer loop (use a boolean flag or an outer-break pattern) to stop
further scanning.
There was a problem hiding this comment.
According to ng_config, there always be a candidate for a group, or any node will always be a candidate of a group.
There was a problem hiding this comment.
@githubzilla Got it — thanks for the clarification. If the local node is always a candidate per ng_config, then guarding native_ng_id behind is_candidate_ is fine.
One small improvement still worth making: break out of both loops as soon as the local node is found (and make the loop variables const). This avoids needless scanning and eliminates any chance of later iterations touching state.
Here’s the minimal change that preserves your current semantics while adding the early breaks:
// check whether this node is in cluster.
for (const auto &pair : ng_configs)
{
const auto &ng_nodes = pair.second;
for (size_t i = 0; i < ng_nodes.size(); ++i)
{
if (ng_nodes[i].host_name_ == local_ip &&
ng_nodes[i].port_ == local_port)
{
node_id = ng_nodes[i].node_id_;
if (ng_nodes[i].is_candidate_)
{
// found native_ng_id.
native_ng_id = pair.first;
}
found = true;
break; // stop scanning this group
}
}
if (found) break; // stop scanning other groups
}Optionally, we can add a debug assert to catch config drift in the future:
assert(ng_nodes[i].is_candidate_ && "Local node should be a candidate per ng_config");If you’re good with this micro-change, I can prepare it as an update to the PR.
✏️ Learnings added
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.624Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.190Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.
githubzilla
force-pushed
the
update_cluster_config
branch
from
f97abd8 to
115bd79
Compare
2 participants
Summary by CodeRabbit
Refactor
Chores
Tests